Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Kusama state version switch and migration. #7015

Merged
merged 11 commits into from
Jul 4, 2023

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Apr 6, 2023

This PR adds state-migration pallet to kusuma to allow switch to trie state version 1.
It should use the same configuration as the ones previously used in westend (#6336)

It may be pertinent to avoid adding this in a runtime upgrade that already have others migration.

@cheme cheme added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. labels Apr 6, 2023
@paritytech-ci paritytech-ci requested review from a team April 6, 2023 09:05
@cheme cheme added E1-database_migration PR introduces code that does a one-way migration of the database. T0-node This PR/Issue is related to the topic “node”. T1-runtime This PR/Issue is related to the topic “runtime”. B1-note_worthy Changes should be noted in the release notes labels Apr 6, 2023
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will enable automatic migration of Kusama after the runtime upgrade is applied?

use sp_std::prelude::*;

/// Initialize an automatic migration process.
pub struct InitMigrate;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add it to the list of migrations.

@paritytech-ci paritytech-ci requested a review from a team April 9, 2023 23:14
@xlc
Copy link
Contributor

xlc commented Apr 9, 2023

Do we have confirmation this is fully compatible with all the ecosystem tooling? e.g smoldot and alternative polkadot clients

@bkchr
Copy link
Member

bkchr commented Apr 9, 2023

Everything that can run Rococo/Westend is already compatible with this change as state version V1 is already running there. Smoldot is also ready AFAIK.

@cheme
Copy link
Contributor Author

cheme commented Apr 10, 2023

So this will enable automatic migration of Kusama after the runtime upgrade is applied?

Yes without privileged account (could be an idea to be able to emergency switch off but that would mean giving a specific power to some account/owner, a bit awkward in my opinion).

@bkchr
Copy link
Member

bkchr commented Apr 10, 2023

In what case would you think that we need an emergency stop?

@cheme
Copy link
Contributor Author

cheme commented Apr 10, 2023

In what case would you think that we need an emergency stop?

Bug, but not much value in it (as can only stop if the blocks are still processing).

@bkchr
Copy link
Member

bkchr commented May 10, 2023

@cheme please merge master.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No major issues, but should fix the comment prior to merge.

type SignedFilter = frame_support::traits::NeverEnsureOrigin<AccountId>;

// Use same weights as substrate ones.
type WeightInfo = pallet_state_trie_migration::weights::SubstrateWeight<Runtime>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be more appropriate to benchmark it properly in this runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, not sure how much change it will be to do so (in theory there is no reason to see significant change: only wasm bin size may interfere a tiny bit maybe).

type Currency = Balances;
type SignedDepositPerItem = MigrationSignedDepositPerItem;
type SignedDepositBase = MigrationSignedDepositBase;
type ControlOrigin = EnsureRoot<AccountId>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure? it would be hard to acquire.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not very comfortable with having a privilege account.
Actually I recently reconsider bringing back the inherent version of the migration (the main issue was the code was a bit everywhere and less cleanly separated, but looking backward, it would have really feel more comfortable to me).

if MigrationProcess::<Runtime>::get() == Default::default() &&
AutoLimits::<Runtime>::get().is_none()
{
AutoLimits::<Runtime>::put(Some(MigrationLimits { item: 160, size: 204800 }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comment about how this number is chosen would be good.

migrations::V0941,
migrations::V0942,
migrations::Unreleased,
init_state_migration::InitMigrate,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why not to move that into migrations::Unreleased,?

migrations::Unreleased migrations do run, we just don't know yet in which of the upcoming Version it will be included.

At release time, merged Unreleased migrations will moved to a new VXXXX migration set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I will do, I just merged to master a bit too fast.
(currently I am trying to get better limit but encounter some limitation with try runtime, but will certainly get new limits soon).

@cheme cheme removed the T0-node This PR/Issue is related to the topic “node”. label May 12, 2023
@cheme cheme changed the title Kusuma state version switch and migration. Kusama state version switch and migration. May 12, 2023
Comment on lines 1487 to 1489
pub type Migrations =
(migrations::V0940, migrations::V0941,
migrations::V0942, migrations::Unreleased);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am no fan of what rustfmt does here.
The list you had before is much more readable.

I would suggest adding #[rustfmt_skip] for this Migrations type as well as for Unreleased and keep one migration / version per line. That also helps with your comment about having this single migration in a release.

Comment on lines 1510 to 1511
pub type Unreleased = (init_state_migration::InitMigrate,);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your comment:

It may be pertinent to avoid adding this in a runtime upgrade that already have others migration.

would be suited here so whoever does the move from Unreleased to VNext is aware of it.
We can then drop the comment.

@cheme
Copy link
Contributor Author

cheme commented May 15, 2023

@bkchr @kianenigma I did change the limits (see https://forum.polkadot.network/t/state-trie-migration/852/11).
Does the weight column looks fine (only in the targetted tab, I am not really use to weight so would appreciate feedback on the value (value is from on_init only).

@@ -2405,7 +2405,11 @@ mod init_state_migration {
if MigrationProcess::<Runtime>::get() == Default::default() &&
AutoLimits::<Runtime>::get().is_none()
{
AutoLimits::<Runtime>::put(Some(MigrationLimits { item: 160, size: 204800 }));
// We use limits to target 600ko proofs per block and
// avg 800_000_000_000 of weight per block.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

800 seconds?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

800_000_000_000 is 800ms I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can lower to 600 (using 3200 instead of 4800 move to 510 blocks the migration time so not taking really longer).

Could make sense to calculate better weight as I believe it is an overestimate (iterating on data since iterator improvement may be cheaper than random data access).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

800ms sounds good! For some reason I thought we measure ref time in nanoseconds, but ty @crystalin

@bkchr
Copy link
Member

bkchr commented Jul 4, 2023

@cheme what is the status here?

@cheme
Copy link
Contributor Author

cheme commented Jul 4, 2023

Forgot to merge the PR :( , it was totally ready iirc, will rebase and merge.

@cheme
Copy link
Contributor Author

cheme commented Jul 4, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for 9f4c073

@cheme
Copy link
Contributor Author

cheme commented Jul 4, 2023

bot merge

@paritytech-processbot
Copy link

Error: Statuses failed for 18cc436

@cheme
Copy link
Contributor Author

cheme commented Jul 4, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 032d3eb into paritytech:master Jul 4, 2023
@cheme
Copy link
Contributor Author

cheme commented Jul 4, 2023

ok, migration is pending in unreleased. @chevdor is it enought to get included in next runtime build?

@chevdor
Copy link
Contributor

chevdor commented Jul 4, 2023

ok, migration is pending in unreleased. @chevdor is it enought to get included in next runtime build?

yes

@cheme
Copy link
Contributor Author

cheme commented Jul 4, 2023

ok, migration is pending in unreleased. @chevdor is it enought to get included in next runtime build?

yes

Ok, if for any reason it ends up not being included in next runtime, then it would be better if:

state_version: 1,

stays at 0 (it breaks warpsync).

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/state-trie-migration/852/13

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. E1-database_migration PR introduces code that does a one-way migration of the database. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants